Remove unused parameters, use CachingWriter, simplify a bit #79
Conversation
| // to write the file using a Writer. | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| properties.store(baos, null); | ||
| String nl = System.lineSeparator(); |
There was a problem hiding this comment.
We need system independent output. Use \n unconditionally.
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| properties.store(baos, null); | ||
| String nl = System.lineSeparator(); | ||
| String output = baos.toString(StandardCharsets.ISO_8859_1) |
There was a problem hiding this comment.
We're leaning pretty heavily on Hyrum's Law here. I'm not convinced we can rely on the description of current encoding in the Javadoc to apply to future encoding. In particular, I would not be at all surprised to see properties.store write in UTF-8 in a future JDK.
There was a problem hiding this comment.
It's been this way since JDK 1.2, so 26 years. And for the encoding, the Properties class has been opened to other encodings in JDK 1.5. The default encoding for reading properties has already been changed in JDK 9. The encoding used here is kinda irrelevant (whether ISO-8859-1 or UTF-8) because all non-ascii chars have been transformed into \uxxxx sequences, thereby removing everything that is encoding specific.
| properties.store(baos, null); | ||
| // The encoding can be either UTF-8 or ISO-8859-1, as any non ascii character | ||
| // is transformed into a \\uxxxx sequence anyway | ||
| String output = baos.toString(StandardCharsets.UTF_8) |
There was a problem hiding this comment.
Probably 8859-1 to avoid the risk of someone slipping in a byte order mark.
| .filter(line -> !line.startsWith("#")) | ||
| .sorted() | ||
| .collect(Collectors.joining("\n", "", "\n")); // system independent new line | ||
| try (Writer pw = new CachingWriter(outputFile, StandardCharsets.UTF_8)) { |
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| class PomPropertiesUtilTest { |
There was a problem hiding this comment.
There's an additional test in the latest draft of my PR you can adopt.
Alternative proposal to #77 as I really don't see the point in reimplementing the
Properties.store()method.